Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PE-814: 2022-01-21 Executive #215

Merged
merged 13 commits into from
Jan 21, 2022
Merged

PE-814: 2022-01-21 Executive #215

merged 13 commits into from
Jan 21, 2022

Conversation

godsflaw
Copy link
Contributor

@godsflaw godsflaw commented Jan 19, 2022 •
edited by brianmcmichael
Loading

Description

PE-814: 2022-01-21 Executive

Contribution Checklist

  • PR title starts with (PE-<TICKET_NUMBER>)
  • Code approved
  • Tests approved
  • CI Tests pass

Checklist

  • Every contract variable/method declared as public/external private/internal
  • Consider if this PR needs the officeHours modifier
  • Verify expiration (4 days monthly and 30 days for the rest)
  • Verify hash in the description matches here
  • Validate all addresses used are in changelog or known
  • Notify any external teams affected by the spell so they have the opportunity to review
  • Deploy spell ETH_GAS="XXX" ETH_GAS_PRICE="YYY" make deploy
  • Verify mainnet contract on etherscan
  • Change test to use mainnet spell address and deploy timestamp
  • Run make date="YYYY-MM-DD" archive-spell to make an archive directory and copy DssSpell.sol, DssSpell.t.sol, and DssSpell.t.base.sol
  • squash and merge this PR

@godsflaw godsflaw self-assigned this Jan 19, 2022
@godsflaw godsflaw marked this pull request as draft January 19, 2022 18:00
@talbaneth
Copy link
Contributor

Need to change the copyright to 2022 here as well :)

talbaneth
talbaneth reviewed Jan 21, 2022
View reviewed changes
src/DssSpell.t.sol Outdated Show resolved Hide resolved
@talbaneth
Copy link
Contributor

Made a small comment about the location of testAAVEDirectBarChange.
Apart from that this is good to deploy.

Verified:
-diffed the spell with goerli
-diffed config.sol with goerli
-diffed all files vs previous spell
-AAVE D3M updates - checked values vs sheet and poll
-reviewed tests
-tests pass

@iamchrissmith
Copy link
Contributor

we should update this to include 2022: https://github.com/makerdao/spells-mainnet/pull/215/files#diff-c9c3998420c475f01de841ca5283ea49ce6c99080a8c6a3fdae77b097bef6778L3

@brianmcmichael brianmcmichael marked this pull request as ready for review January 21, 2022 16:02
@brianmcmichael
Copy link
Contributor

Copyright dates updated at e11b200

@iamchrissmith
Copy link
Contributor

confirmed changes against goerli and Aave D3M against executive spreadsheet. I have a question about the verification code, but other than that looking good.

test are running (and have been for about an hour...)

@talbaneth
Copy link
Contributor

Hash confirmed.
Tests passing:

dapp-test: rpc block: latest
Running 13 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testFail_notScheduled() (gas: 7236)
[PASS] test_auth_in_sources() (gas: 281473904825281)
[PASS] test_use_eta() (gas: 326431)
[PASS] testOnTime() (gas: 670795)
[PASS] test_nextCastTime() (gas: 327273)
[PASS] testFailTooEarly() (gas: 3457)
[PASS] test_bytecode_matches() (gas: 1190042)
[PASS] testAAVEDirectBarChange() (gas: 674578)
[PASS] test_auth() (gas: 281473912910764)
[PASS] testSpellIsCast_GENERAL() (gas: 7784572)
[PASS] testFailWrongDay() (gas: 3699)
[PASS] testCastCost() (gas: 672681)
[PASS] testFailTooLate() (gas: 3853)

talbaneth
talbaneth previously approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@talbaneth talbaneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for deployment.

@talbaneth
Copy link
Contributor

dapp-test: rpc block: latest
Running 13 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testFail_notScheduled() (gas: 12236)
[PASS] test_auth_in_sources() (gas: 281473904825312)
[PASS] test_use_eta() (gas: 333431)
[PASS] testOnTime() (gas: 680295)
[PASS] test_nextCastTime() (gas: 334273)
[PASS] testFailTooEarly() (gas: 8457)
[PASS] test_bytecode_matches() (gas: 1199542)
[PASS] testAAVEDirectBarChange() (gas: 684078)
[PASS] test_auth() (gas: 281473912910796)
[PASS] testSpellIsCast_GENERAL() (gas: 8981391)
[PASS] testFailWrongDay() (gas: 8699)
[PASS] testCastCost() (gas: 682181)
[PASS] testFailTooLate() (gas: 8853)

@brianmcmichael
Copy link
Contributor

With deployed address:

dapp-test: rpc block: latest
Running 13 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testFail_notScheduled() (gas: 12236)
[PASS] test_auth_in_sources() (gas: 281473904825312)
[PASS] test_use_eta() (gas: 333431)
[PASS] testOnTime() (gas: 680055)
[PASS] test_nextCastTime() (gas: 334273)
[PASS] testFailTooEarly() (gas: 8457)
[PASS] test_bytecode_matches() (gas: 1199542)
[PASS] testAAVEDirectBarChange() (gas: 683838)
[PASS] test_auth() (gas: 281473912910796)
[PASS] testSpellIsCast_GENERAL() (gas: 8981151)
[PASS] testFailWrongDay() (gas: 8699)
[PASS] testCastCost() (gas: 681941)
[PASS] testFailTooLate() (gas: 8853)

@iamchrissmith
Copy link
Contributor

tests with address pass at block number 14050237

Running 13 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testFail_notScheduled() (gas: 12236)
[PASS] test_auth_in_sources() (gas: 281473904825312)
[PASS] test_use_eta() (gas: 333431)
[PASS] testOnTime() (gas: 680515)
[PASS] test_nextCastTime() (gas: 334273)
[PASS] testFailTooEarly() (gas: 8457)
[PASS] test_bytecode_matches() (gas: 1199542)
[PASS] testAAVEDirectBarChange() (gas: 684298)
[PASS] test_auth() (gas: 281473912910796)
[PASS] testSpellIsCast_GENERAL() (gas: 8981611)
[PASS] testFailWrongDay() (gas: 8699)
[PASS] testCastCost() (gas: 682401)
[PASS] testFailTooLate() (gas: 8853)

talbaneth
talbaneth approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@talbaneth talbaneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to merge.

@brianmcmichael brianmcmichael merged commit 464a7eb into master Jan 21, 2022
@brianmcmichael brianmcmichael deleted the PE-814 branch January 21, 2022 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamchrissmith iamchrissmith iamchrissmith approved these changes

@talbaneth talbaneth talbaneth approved these changes

@brianmcmichael brianmcmichael Awaiting requested review from brianmcmichael

Assignees

@godsflaw godsflaw

Labels
Priority: High Status: Work In Progress Type: Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@godsflaw @talbaneth @iamchrissmith @brianmcmichael